CR.Name must be the CSI driver name, so field will duplicate that information?

Also, CR.Spec.DriverConfig.DriverName seems a bit verbose/redundant.

^ this is a good point IMO, I don't really understand the purpose of the new DriverName field.

yeah I am going to rename it. But we do need a field here as a union discriminator which will basically make it obvious that if driverName: vsphere.vmware.com then - only vsphere will be set.

https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#discriminated-unions

I am torn about renaming driverName to Name. Elsewhere in OCP docs, union discriminators are called Type - if we do that, then we probably have to define a new enum.

Ah, I don't have a strong preference on renaming the field, I think DriverName is fine. I just didn't understand the purpose, but that doc on discriminated unions explains it.

Few nits on this:

• The first word should match the json tag
• We should use the standard wording that other APIs use for when this is optional
• You only need one optional tag, we prefer +optional
• You shouldn't use a pointer here unless you have a good reason to do so, do you need a pointer?

I was reading about API conventions and although I have accepted this suggestion, one of the things which I find sometimes problematic is - CSIDriverConfigSpec says DriverName is a required field and hence by that definition - an empty CSIDriverConfigSpec isn't a valid object because DriverName is not set. I know on the wire, if field is empty - it is entirely omitted but I think that after deserializing we basically have a GO object that is essentially invalid and does not conform to schema it defines itself, which is weird.

So I think for union types which have a required union discriminator I think that a nil pointer is only sensible default if users don't want to specify one.

It's a trade off we have decide to make for the discoverability benefits. But yes, you need to teach your controller to handle the empty DriverConfig. It shouldn't be too difficult and isn't dissimilar to the nil check you'd have to otherwise do

emptyConfig := operatorv1.CSIDriverConfigSpec{} 
if spec.DriverConfig == emptyConfig {
  // No driver config was specified
  return nil
}

Can you clarify what this means? It doesn't make sense to me, do you mean resources such as?

Added more details. PTAL

Where does this configuration live? On the Infrastructure object?

Yes - basically what @jcpowermac is proposing in his enhancement.

Ok, I'd expect the comment here to mention that it's on the Infrastructure object, gives user a hint as to where they can find the defaults

done. PTAL

This should be an enum, do we have a list of valid values for this?

I have renamed this to CSIDriverType and defined a new enum. @bertinatto raised a concern that we were overloading CSIDriverName type which is used for defining driver name.

What value must DriverName have for this configuration to be observed?

Updated to reflect the values.

As this enum is expected over time, you need to add a developer note to explain what consumers should do when they observe an unknown value, there's typically two options there

Typically the closer object to the action (this resource) would take precedence, it seems odd to say the Infrastructure object will take precedence here, I would expect to reverse that.

Having the closer object allows whoever owns that specific component to override the global configuration should they need to.

Why are you suggesting it to be this way around?

Currently its bit like you would expect ebs provisioner as default in a storageclass deployed on AWS cluster not ceph provisioner. One can deploy ceph on OCP AWS cluster sure but it requires bunch of manual configuration.

Similarly topology from Infrastructure object is guaranteed to be right and does not require any further manual configuration in vcenter, so we want to start with somewhat stricter definition of topologyCategories if Infra object already defines one.

Currently we are thinking - once Infra changes are merged, we will not allow setting these values at all by end users and any changes to ClusterCSIDriver topologies will be rejected by webhook, if Infra object already defines a topology(via failure-domains). I will update the wording here to be more clear on this.

But we also realize that, compute and storage topologies can have two different values - so we don't want to close the door completely on that option, so we may choose to relax this in a future version of OCP.

If the right CSIDriverType is "VSphere", then this must be json:"vSphere

Check the discriminated unions in the config v1 types and follow their patterns

fixed.